Skip to content

Testing Components with TestBed #1061

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

justindujardin
Copy link

@justindujardin justindujardin commented Oct 27, 2017

This adds support for jasmine/mocha angular testing using the TestBed class exported by Angular.

Fixes: #752, #479, #606

  • Zone.js loads appropriate patches for jasmine/mocha
  • Existing tests switched to TestBed from TestApp
  • TestBed specs passing
    • ListView
    • ModalDialog
    • Platform Filter Components
    • Third Party Components
    • Renderer E2E
    • Renderer createElement
    • Renderer attach/detach
    • Renderer lifecycle
    • DetachedLoader

@ghost ghost added the ♥ community PR label Oct 27, 2017
@justindujardin
Copy link
Author

justindujardin commented Oct 27, 2017

@vakrilov Most of the groundwork is done for TestBed. I'm in the process of updating the existing tests that use TestApp to use TestBed instead.

There are two tests that currently fail that I could use feedback on. I left TODO comments describing the issues:

@NativeScript NativeScript deleted a comment from ns-bot Oct 28, 2017
@NativeScript NativeScript deleted a comment from ns-bot Oct 28, 2017
@NativeScript NativeScript deleted a comment from ns-bot Oct 28, 2017
@NativeScript NativeScript deleted a comment from ns-bot Oct 28, 2017
@NativeScript NativeScript deleted a comment from ns-bot Oct 28, 2017
@NativeScript NativeScript deleted a comment from ns-bot Oct 28, 2017
@NativeScript NativeScript deleted a comment from ns-bot Oct 28, 2017
@NativeScript NativeScript deleted a comment from ns-bot Oct 28, 2017
@NativeScript NativeScript deleted a comment from ns-bot Oct 28, 2017
@NativeScript NativeScript deleted a comment from ns-bot Oct 28, 2017
@justindujardin
Copy link
Author

I think this is ready for review now. 👍

Notes

  • I intended to write specs to verify TestBed, but it seemed a little weird testing the testing setup, and it gets pretty thorough testing from the existing specs in the nativescript-angular codebase.
  • I also intended to remove the TestApp class, but there are router tests that use it and assert about the app. It seems an appropriate fixture for that test, so it gets to hang around.

@justindujardin justindujardin changed the title Testing Components with TestBed (WIP) Testing Components with TestBed Oct 31, 2017
@vakrilov
Copy link
Contributor

vakrilov commented Nov 1, 2017

Kudos for this massive PR!!!
Running some CI builds:

Run ci
e2e

@justindujardin
Copy link
Author

@vakrilov I noticed some of the Jenkins tests failed when you ran the ci job, but I can't access the logs. I'm happy to debug any issues if you can help me understand how to reproduce them locally. Whenever you have some time, I'm not in a rush.

@vchimev
Copy link
Contributor

vchimev commented Nov 7, 2017

renderer-android

@vchimev
Copy link
Contributor

vchimev commented Nov 7, 2017

renderer-ios

 - update zone build and add jasmine/mocha test scripts, see: NativeScript/zone.js#1
 - refactor testing and zone-js modules to work with each other
 - add generous TestBed setup helpers in testing/src/util.ts
 - add single import test helpers for zone-js patches in zone-js/testing.[framework].ts files
 - update peer deps to reflect updated zone (could peer dep be dropped since prebuilts are exported?)

chore: disable most tests and enable ones that use TestApp one-by-one
 - add test entry point script that inits the test environment for TestBed
 - list view, modal dialog pass
 - detached-loader and platform-filter-component could use feedback. see todos

chore: replace the remaining TestApp usages in test suite

 - xdescribe the failing tests.
 - I think the remaining problems boil down to `dumpView` indicating the ComponentFixture comes back with the root components, and `@ViewChild` not finding DetachedLoader by its class.
 - remove some duplication in testing utilities

chore: cleanup and remove some diff noise from a few tests

chore: remove more test noise

 - are the line-endings different on this file? :(

chore: convince dumpView and TestComponentRenderer to agree on things

 - all the TestBed tests except for the DetachedLoader ones and a single Renderer lifecycle are passing.
 - update NativeScriptRenderer.selectRootElement to find views by ID when given a selector of an id string. TestBed uses this when creating componentRefs to get at the correct views.
 - change NativeScriptTestComponentRenderer to inject only a ProxyViewContainer which mimics what TestApp did.
 - update dumpView to strip off the new "source" data attached to a view.toString() result.

chore: cleanup lint

chore: make nTestBed helpers automatically clean up test components

 - before the components were destroyed by TestBed, but not removed from the rootView.
 - maintain a list of active fixtures for a set of tests, and remove them all when the tests complete.
 - reorder to the testing utils to flow better when reading (start with test init, then before/after then render components)
 - clean up some lint.

chore: fix issue where nTestBedBeforeEach overwrote its own imports

 - Fixes the DetachedLoader tests, and makes them MUCH simpler.
 - When you configure the test bed module, you need to specify a full list of imports, because they completely overwrite the imports array that is used.
 - That's yet another reason to use the provided helper functions, they merge in the common {N} imports for you.
 - ... and some lint cleanup

chore: make renderer lifecyle test more robust

 - the first assertion is that the view after init has been called. rather than assert it, just wait for it using an observable and avoid asserting about timing and implementation specific details of the system. Specifically this removes the assumption that `app.tick()` will advance time and call `ngAfterViewInit` on the component.

chore: cleanup from review

 - re-enable all tests in karma.conf.js
 - remove some diff noise
@hypery2k
Copy link
Contributor

hypery2k commented Jan 2, 2018

@vakrilov is it possible to also back port this one to nativescript-angular 4.x?

Would be great to re invoke the Jenkins checks

@vakrilov
Copy link
Contributor

vakrilov commented Jan 8, 2018

Run ci
e2e

/**
* Perform basic TestBed environment initialization. Call this once in the main entry point to your tests.
*/
export function nTestBedInit() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you prefix the all test method with ns instead of n.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, what about:

export function testingRootView(): View
export function promiseWait(durationMs: number)

 - incorporate changes from zone to include @panayot.cankov's drainMicroTaskQueue update
 - update zone.js deps to reference "*" for version. Ideally it would be removed, but I believe @angular has a peer dependency on it so satisfy it with a star. This is because zone.js files are prebuilt specifically for {N} so there is no need to bring them in as a dep.
@ghost ghost added the ♥ community label Jan 9, 2018
 - the TestBed changes do not depend on the upgrade (after review), and given that there are strange errors, prefer to reduce the number of changes that are not strictly required.
@hypery2k
Copy link
Contributor

@vakrilov can you retrigger the Jenkins build?

@sis0k0
Copy link
Contributor

sis0k0 commented Jan 10, 2018

run ci

@justindujardin
Copy link
Author

Hi, there are some problems with bootstrapping the test app in @angular ~5.1.0 that I'm debugging. I'll update here when I think it's worth running the CI build again.

@sis0k0
Copy link
Contributor

sis0k0 commented Jan 10, 2018

@justindujardin, you can target Angular 5.2.0, it'll make rebasing easier.

@hypery2k
Copy link
Contributor

@justindujardin Any chance for you to update your PR?

@abdulhaq-e
Copy link

Hey @justindujardin , any updates on this PR ?

Thanks a lot, great work.

@hypery2k
Copy link
Contributor

hypery2k commented Feb 4, 2018

@abdulhaq-e I've updated his branch, see #1175

@abdulhaq-e
Copy link

@hypery2k Nice! have your tried it out?

Also, why haven't the tests completed in your PR?

@SmailHammour
Copy link

When will this be available?

@justindujardin
Copy link
Author

Sorry, it looks like I won't have free time to continue work on this problem in the short term.

At this point perhaps @vakrilov can find a team member to resolve the conflicts and debug the tns test ios startup failure that happens with angular 5 (a guess, it didn't happen in the original PR for 4.x)?

The code and tests here should still be 100% okay.

@ndcunningham
Copy link

any update on this?

@ns-bot
Copy link

ns-bot commented Apr 20, 2018

Can one of the admins verify this patch?

@AresDev
Copy link

AresDev commented Apr 25, 2018

This is really a need, any update for this?

@hypery2k
Copy link
Contributor

i think we can close this PR, as we having #1175

@vakrilov
Copy link
Contributor

Closing in favor of #1175

@vakrilov vakrilov closed this May 10, 2018
@ghost ghost removed the ♥ community PR label May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants